Lint GitHub Actions workflows with actionlint - #13083
Conversation
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Review of draft PR — since this is explicitly seeking feedback:
The SC2086 quoting fixes in maven.yml are correct and should be uncontroversial.
The new actionlint.yml workflow is well structured — proper ASF header, path filtering, concurrency group, minimal permissions, persist-credentials: false. A few observations:
-
Docker image pinning: The comment in the workflow already acknowledges this, but to be explicit — the project convention (visible in
maven.yml) is SHA-pinned actions (uses: actions/checkout@3d3c42e5aac5...). Fordocker://images the equivalent is a digest pin:docker://rhysd/actionlint@sha256:.... Before merging, this should be aligned with the project convention. -
Trigger scope vs.
maven.yml: Thepushtrigger has nobranchesfilter (unlikemaven.ymlwhich restricts tomaster), meaning actionlint runs on every branch push touching workflows. Thepull_requesttrigger similarly has nobranchesfilter. This is probably intentional for a linting workflow, but it's a different scope than the main CI — worth an explicit note in the workflow comment if kept. -
Regarding the draft questions — the shared-workflow approach (
maven-gh-actions-shared) seems like the right long-term home if multiple Maven repos should adopt this, since it avoids duplicating the workflow definition. This standalone version is good as a proof of concept.
Overall: clean PR, sensible addition. The quoting fixes are independently mergeable.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
| # pin their actions by SHA. | ||
| - name: Run actionlint | ||
| uses: docker://rhysd/actionlint:1.7.12 | ||
| with: |
There was a problem hiding this comment.
Per project convention, this should be pinned by image digest rather than version tag:
| with: | |
| uses: docker://rhysd/actionlint@sha256:<digest> |
You can find the digest with:
docker pull rhysd/actionlint:1.7.12
docker inspect --format='{{index .RepoDigests 0}}' rhysd/actionlint:1.7.12
The code comment at line 52-54 already flags this — just leaving a concrete suggestion for the final version.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds a GitHub Actions workflow that lints other workflows using actionlint, and fixes the initial shellcheck findings it reported in the existing Maven workflow.
Changes:
- Add
.github/workflows/actionlint.ymlto runactionlinton workflow changes (push + PR), with concurrency cancellation. - Fix shellcheck SC2086 findings in
.github/workflows/maven.ymlby quoting$GITHUB_ENV/$GITHUB_PATH.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| .github/workflows/maven.yml | Quotes $GITHUB_ENV/$GITHUB_PATH in two steps to satisfy shellcheck/actionlint. |
| .github/workflows/actionlint.yml | Introduces a new CI workflow to lint .github/workflows/** with actionlint. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout maven | ||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 |
9ebc1ae to
3c0e035
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Follow-up review after force-push — the approach has changed significantly.
Good rework. Switching from a standalone docker://rhysd/actionlint image to github/super-linter/slim is a pragmatic solution: it satisfies the ASF allowed-actions policy (GitHub-owned), gives you actionlint + shellcheck in one step, and the SHA pin (b807e99d... = v7) follows the repo convention. The workflow-level comment explaining the rationale is helpful.
A few observations on the new version:
-
github/super-linterfork freshness: Thegithub/super-linterfork (v7, Aug 2024) lags the upstreamsuper-linter/super-linterby 1300+ commits — that's two years of actionlint updates, bug fixes, and linter version bumps. If the ASF policy were ever relaxed to allowsuper-linter/super-linter, switching would be trivial. For now the trade-off is fine, just worth noting. -
pushtrigger scope: Still nobranchesfilter onpush(unlikemaven.ymlwhich restricts tomaster). As noted in the previous review, this means the workflow runs on every branch push touching.github/workflows/**, including contributor forks. This is arguably desirable for a linting workflow (early feedback), but it's a departure frommaven.yml's pattern — the PR description acknowledges this is intentional. -
Previous review findings addressed:
- ✅ Image pinning → now SHA-pinned
github/super-linter/slim@b807e99d... - ✅ Permissions →
contents: readinstead of the old{} - ✅ Checkout SHA → matches
maven.yml(3d3c42e5...=v7.0.1)
- ✅ Image pinning → now SHA-pinned
-
The SC2086 quoting fixes in
maven.ymlare correct and independently mergeable.
No blocking issues found. The draft questions in the PR description (standalone vs. shared repo, advisory vs. required check) are the right discussion points for the community.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| env: | ||
| VALIDATE_ALL_CODEBASE: true | ||
| VALIDATE_GITHUB_ACTIONS: true | ||
| DEFAULT_BRANCH: master |
| - name: Lint workflows with actionlint (via super-linter) | ||
| uses: github/super-linter/slim@b807e99ddd37e444d189cfd2c2ca1274d8ae8ef1 # v7 | ||
| env: | ||
| VALIDATE_ALL_CODEBASE: true |
3c0e035 to
5854606
Compare
|
Update after review — reworked, and now green on CI. The original
CI is green now ✅. This also resolves the earlier feedback: @gnodet's pinning note (now SHA-pinned) and the Copilot Bonus: super-linter bundles shellcheck/yaml/etc., so coverage can be extended later via more Supporting data: I ran actionlint across 121 locally-checked-out Maven repos — 17 have ≥1 finding (86 total). ~79% are |
There was a problem hiding this comment.
🟡 Changes recommended
The new workflow hard-codes DEFAULT_BRANCH: master, which can cause incorrect behavior on forks with different default branch names and undermines the stated goal of early feedback for contributors on forks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| env: | ||
| VALIDATE_ALL_CODEBASE: true | ||
| VALIDATE_GITHUB_ACTIONS: true | ||
| DEFAULT_BRANCH: master | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Incremental review after rebase + fetch-depth: 0 addition.
The only delta since the previous review is adding fetch-depth: 0 — makes sense, super-linter uses the commit range for its diff mode. With VALIDATE_ALL_CODEBASE: true it is less critical (all files are linted regardless), but it avoids a warning from super-linter about shallow clones, so no objection.
Re Copilot's latest flag on DEFAULT_BRANCH: master: the repo's default branch is master, so the value is correct. That said, the super-linter docs note that on GitHub Actions it auto-detects the default branch, so the explicit DEFAULT_BRANCH env var could simply be removed — it's unnecessary and removing it would also sidestep the fork concern Copilot raised.
No new issues beyond what was noted in the previous reviews.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Re-review after latest push (commit 5854606)
The only delta since the previous review is fetch-depth: 0 on the checkout step, with a comment stating super-linter needs full history to resolve the commit range.
One observation:
fetch-depth: 0vsVALIDATE_ALL_CODEBASE: true: WithVALIDATE_ALL_CODEBASE: true, super-linter lints every matching file in the repo — it doesn't diff againstDEFAULT_BRANCH, so there's no commit range to resolve and a shallow clone would suffice. Thefetch-depth: 0becomes relevant whenVALIDATE_ALL_CODEBASEis set tofalse(lint only changed files, which requires the merge-base). If the intent is to lint all workflow files on every run, the full clone is unnecessary overhead; if the intent is to eventually switch to changed-files-only (which makes more sense for a large repo), thenVALIDATE_ALL_CODEBASEshould befalse. Either way, one of the two settings is mismatched.
No other changes. Previous review observations (push trigger scope, super-linter fork freshness) still apply.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
5854606 to
d23dc7e
Compare
This adds a small CI job running actionlint over
.github/workflows/**— it checks workflow syntax, expression contexts, and embedded shell (via shellcheck). We have no workflow linting today.It triggers on any branch push and any PR (path-filtered to
.github/workflows/**), so contributors get early feedback even on their forks, before a PR exists. Aconcurrencygroup cancels superseded runs on the same ref.On its first run it flagged 4 real issues — unquoted
$GITHUB_ENV/$GITHUB_PATH(SC2086) inmaven.yml's two "Extract Maven distribution" steps — fixed in the second commit.Opening as a draft to gather opinions before committing to it — @slawekjaranowski (and others):
apache/maven?apache/maven-gh-actions-shared(so every Maven repo adopts it over time via a small caller workflow), rather than a standalone workflow per repo? This PR is the standalone demo; happy to move it to the shared repo if that's preferred.docker://image (digest-pinned for a final version) vsreviewdog/action-actionlintvs a pinned download — preference?Note: actionlint does not flag the
if: ${{ ... }}pattern, so that stays as-is.